Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow using default global from #35

Closed
wants to merge 3 commits into from

Conversation

spawnia
Copy link
Contributor

@spawnia spawnia commented Dec 3, 2024

The Laravel mail configuration defines a global from address by default, see https://github.com/laravel/laravel/blob/0993d09dc8206d0933628074036427344be16fc5/config/mail.php#L100-L114. Thus, I think it is unnecessary and redundant to force users of this package to repeat this setting specifically for the microsoft-graph mailer.

The Laravel mail configuration defines a global from address by default, see https://github.com/laravel/laravel/blob/0993d09dc8206d0933628074036427344be16fc5/config/mail.php#L100-L114.
Thus, I think it is unnecessary and redundant to force users of this package to repeat this setting specifically for the `microsoft-graph` mailer.
@spawnia
Copy link
Contributor Author

spawnia commented Apr 7, 2025

@geisi Could you take a look? Removing the requirement for an extra from would simplify our configuration.

@geisi
Copy link
Contributor

geisi commented Apr 7, 2025

@spawnia

Thanks for submitting this PR!

I understand the goal of reducing redundancy by relying on global mail.from setting, I can't merge this change.

Removing this configuration requirement, even if falling back to the global default, is a breaking change. It would break setups for existing users who have configured the from address specifically for this mailer as currently required. We need to maintain backward compatibility within the current version line.

We might revisit this as an optional setting in a future major version, but we must keep the explicit configuration for now.

Thanks again for your contribution and understanding!

@geisi geisi closed this Apr 7, 2025
@spawnia
Copy link
Contributor Author

spawnia commented Apr 7, 2025

I don't see how removing the requirement for explicit configuration is a breaking change. Overriding the global default is still possible, just not necessary anymore. I only removed a single line of productive code that checks for the existence of the explicit setting, not any code that would use it.

Should I make it more apparent by adding an explicit test to show how from can be overridden? And perhaps mention in the README how both options are possible?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants